Skip to content

Conversation

@Hweinstock
Copy link
Contributor

Problem

Importing from .. or an index.ts file can lead to circular dependencies.

Solution

  • add a lint rule to discourage importing from ...
  • migrate existing cases to import directly from the target module.

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

This comment was marked as off-topic.

@Hweinstock Hweinstock marked this pull request as ready for review January 15, 2025 15:15
@Hweinstock Hweinstock requested review from a team as code owners January 15, 2025 15:15
'Avoid child_process, use ChildProcess from `shared/utilities/processUtils.ts` instead.',
},
{
name: '..',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents importing from index.ts files in other paths? E.g. ./adjacentFolder/folderWithIndex/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but I think the circular dependency is only a problem when the index.ts is importing from the current file as well. This should only happen when the index.ts is a parent rather than an adjacent folder.

There is another case where the index.ts is more than one level up, but I wasn't able to find any instances of this in the toolkit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found the most common pitfall to be having a file that depends on an index.ts that is exporting something depends on your file. E.g.

// connection.ts
import { logger } from './logger' // index.ts import
// logger/index.ts
export logger from './logger'
export loggerUtil from './loggerUtil'
// logger/loggerUtil.ts
import { connectionType } from '../connection'

connection.ts > logger/index.ts > loggerUtils.ts > connection.ts,
when in reality we just want connection.ts > logger/logger.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yeah unfortunately, this won't address that. I would also imagine it would be hard to catch via lint rule.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution!

@justinmk3 justinmk3 merged commit 0d7ea7d into aws:master Jan 15, 2025
25 of 26 checks passed
@Hweinstock Hweinstock deleted the lint/indexImports branch January 15, 2025 17:29
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
## Problem
Importing from `..` or an `index.ts` file can lead to circular
dependencies.

## Solution
- add a lint rule to discourage importing from `..`. 
- migrate existing cases to import directly from the target module.
kevluu-aws pushed a commit to kevluu-aws/aws-toolkit-vscode that referenced this pull request Jan 23, 2025
## Problem
Importing from `..` or an `index.ts` file can lead to circular
dependencies.

## Solution
- add a lint rule to discourage importing from `..`. 
- migrate existing cases to import directly from the target module.
s7ab059789 pushed a commit to s7ab059789/aws-toolkit-vscode that referenced this pull request Feb 19, 2025
## Problem
Importing from `..` or an `index.ts` file can lead to circular
dependencies.

## Solution
- add a lint rule to discourage importing from `..`. 
- migrate existing cases to import directly from the target module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants